-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance domain-picker package encapsulation #43045
Enhance domain-picker package encapsulation #43045
Conversation
domain-picker change found in: #43045
domainSearch: string; | ||
|
||
/** Called when the domain search query is changed */ | ||
onSetDomainSearch: ( value: string ) => void; | ||
} | ||
|
||
const PAID_DOMAINS_TO_SHOW = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is defined also at packages/domain-picker/src/hooks/constants.ts
?
@@ -0,0 +1,3 @@ | |||
export const PAID_DOMAINS_TO_SHOW = 5; | |||
export const selectorDebounce = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see selectorDebounce
used anywhere?
@@ -0,0 +1,3 @@ | |||
export const PAID_DOMAINS_TO_SHOW = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be 9 for the modal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how it is on master
.
} | ||
|
||
// Another pulsing animation for placeholders | ||
@keyframes loading-fade { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's prefix this with something because keyframes end up in global scope and end up colliding with other with same name.
@@ -188,6 +188,7 @@ $domain-picker__suggestion-item-height: 24px; | |||
} | |||
|
|||
&.placeholder { | |||
color: red; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗️ :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha. This is a remnant of me trying to make sure the CSS is taking effect. Thanks :)
packages/domain-picker/package.json
Outdated
"private": true | ||
"private": true, | ||
"devDependencies": { | ||
"@wordpress/base-styles": "1.9.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely needs ^
packages/domain-picker/package.json
Outdated
"private": true | ||
"private": true, | ||
"devDependencies": { | ||
"@wordpress/base-styles": "1.9.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an actual dependency, not devDependency
? 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a devDep because it’s SASS. Will never make it to production. And the lib docs recommended adding it as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point in the monorepo all devDependencies went to root package.json
— I don't actually know how it is nowadays @Automattic/team-calypso ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right when it comes to client
folder and any sub-project that is not a package. Packages are exempt from this because they're supposed to be standalone and independent of calypso.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to raise this point before, but it's a bit irrelevant as long as this is a private package. With the new yarn setup, I believe we no longer have the devDependencies only at the root restriction we did with npm.
That said, this should be a dependency not a dev dependency. The package prepare scripts produce assets in the dist
folder. These assets import sass files which will depend on the package:
This package can't be used without those assets being present. If those assets were bundled into CSS at prepare, this would be a dev dependency, but that's not the case.
@@ -35,6 +29,10 @@ const DomainPickerCategories: React.FunctionComponent< Props > = ( { | |||
onSelect( slug ); | |||
}; | |||
|
|||
const domainCategories = useSelect( ( select ) => | |||
select( 'automattic/domains/suggestions' ).getCategories() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we follow the pattern to register the store to get the key and then use to key to select data from the store? Like here: https://github.com/Automattic/wp-calypso/pull/43045/files#diff-0d151bc68b92f69ebc364c9a4b674116R9-R11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a problem with the domains suggestions store because it requires configuration on registration. A shared component shouldn't need to concern itself with application configuration.
Expecting the store to be registered by the application the store correctly and using the string key directly seems to be the best.
We could consider some defensive practices, like throwing a helpful error if the store is required but not found:
Error: The domain-picker component expected `automattic/domains/stores` to be registered but it was not found. Please register the store before this component is used. See some-link-to-documentation.
const DOMAIN_SUGGESTIONS_STORE = DomainSuggestions.register( { | ||
vendor: DOMAIN_SUGGESTION_VENDOR, | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to #43045 (comment) we should register/configure the store here but let the application (Gutenboarding/FSE) do this and assume it has been already registered when accessing it directly via the store key inside the shared package.
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~8 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
type DomainSuggestion = import('@automattic/data-stores').DomainSuggestions.DomainSuggestion; | ||
type DomainSuggestionQuery = import('@automattic/data-stores').DomainSuggestions.DomainSuggestionQuery; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these may have reappeared in a rebase of previous PR. I attempted to remove all of this type of type import in favor of import type
.
It would be nice to restore that:
import type { DomainSuggestions } from '@automattic/data-stores';
type DomainSuggestion = DomainSuggestions.DomainSuggestion;
type DomainSuggestionQuery = DomainSuggestions.DomainSuggestionQuery;
That can be done in another PR.
Let's get #43046 merged here so we can review a fully functional PR against master. |
packages/domain-picker/package.json
Outdated
@@ -39,7 +39,8 @@ | |||
"lodash": "^4.17.15", | |||
"tslib": "^1.10.0", | |||
"use-debounce": "^3.1.0", | |||
"uuid": "^7.0.2" | |||
"uuid": "^7.0.2", | |||
"@wordpress/base-styles": "^1.9.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we're using ^1.8.0
elsewhere. Let's match that version.
"@wordpress/base-styles": "^1.9.0" | |
"@wordpress/base-styles": "^1.8.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK these are equivalent due to the caret. But I'll make them exactly the same.
1. No need for extenal stying to look as it should 2. No more coupling with client folder 3. No more coupling with Gutenboading (S)CSS variables 4. Move as much as possible domain-picker logic to domain-picker 5. Use stores instead of props when possible (domainCategories) 6. Clean up a lot of uneeded CSS
* Adapt to new domain-picker changes domain-picker change found in: #43045 * Fix typo in analyticsUiAlgo * Fix handler name and self-close DomainPicker tag
a1561d8
to
bbfd0bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One regression I've found is about not being able to clear the domain query determined by the site title value. Demo: https://cloudup.com/cE_AhsthdW4
A possible solution I see is the suggestion I've made on the other PR: #43046 (comment)
LE: also related to this, with every key pressed in the domain input, the value seems to be refreshed, some of the keys pressed are ignored and the UI feels a bit sluggish compared to production.
|
||
const { domainSearch, domainCategory } = useSelect( ( select ) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After moving domainCategory
state into DomainPicker
component, the selected domain category is no longer persisted after closing Domain Modal. Is this something we want? cc: @dubielzyk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently in production: after I've selected a domain category and then confirm de selection, next time I open the Domain Modal, I find that category already selected.
On this branch: category selection isn't saved from one DomainModal interaction opening to another: https://cloudup.com/cgXioTPqRx6
- Cleanup types and props in DomainPicker. - Don't debouce domain input value update.
- Use initialDomainSearch prop to initialize domainSearch state.
@@ -251,7 +251,7 @@ const DomainPicker: FunctionComponent< Props > = ( { | |||
<SuggestionNone /> | |||
) ) } | |||
{ ! paidSuggestions && | |||
times( quantity - 1, ( i ) => <SuggestionItemPlaceholder key={ i } /> ) } | |||
times( quantity, ( i ) => <SuggestionItemPlaceholder key={ i } /> ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't seem to be well supported by the placeholder items causing a jump: https://cloudup.com/c9q7B6BXjrx
LE: This change is ok to fix the popover jump. Please check #43045 (comment) to fix the DomainModal issue mentioned here.
export const DOMAINS_SUGGESTION_COUNT_IN_POPOVER = 5; | ||
export const DOMAINS_SUGGESTION_COUNT_IN_MODAL = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure these new constants are needed in Gutenboarding.
We already know we want a single free domain and there is PAID_DOMAINS_TO_SHOW
defined in this file.
Then there is the bigger value (DOMAINS_SUGGESTION_COUNT_IN_MODAL
) which should be handled internally by the DomainPicker package. According to latest changes, even the Modal will display 6 items by default and then just double that amount.
domainSearch.trim(), | ||
domainCategory, | ||
useI18n().i18nLocale, | ||
10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 | |
quantity + 1 |
This will fix the jump mentioned in #43045 (comment)
Changes proposed in this Pull Request
It includes changes from #43046 merged into it and the following.
This PR makes domain-picker more FSE friendly and hopefully useable anywhere.
Changes are:
Testing instructions
This PR isn't testable by itself. It goes hand in hand with #43046. Please follow testing instructions there.#43046 was merged into this one.